Hardening: Avoid shell-based popen usage in InspectFile operator#3489
Hardening: Avoid shell-based popen usage in InspectFile operator#3489praneet390 wants to merge 4 commits intoowasp-modsecurity:v3/masterfrom
Conversation
|
Context: This PR comes from a security review of the @inspectFile operator execution path. While current data flow does not allow command injection due to engine-controlled inputs (e.g., FILES_TMPNAMES via mkstemp), the popen + concatenated string pattern is shell-based and fragile by design. Opening this draft PR to discuss a possible migration toward argv-based exec/spawn for defense-in-depth and safer future semantics across v2 and v3. Happy to implement the change following maintainer guidance on preferred cross-platform approach. |
|
do you want to work on this PR? Actually, there are a few lines (which are comments) were added, so I don't see any relevant changes. |
|
|
@airween |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| * ModSecurity, http://www.modsecurity.org/ | ||
| * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 | ||
| */ | ||
|
|
There was a problem hiding this comment.
The file already contains the full ModSecurity/Apache 2.0 license header at the top. This additional shorter license block is redundant and can create confusion/inconsistency across the codebase; please remove the duplicated header and keep the existing canonical one only.
| /* | |
| * ModSecurity, http://www.modsecurity.org/ | |
| * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. | |
| * | |
| * Licensed under the Apache License, Version 2.0 | |
| */ |
| // Create mutable copies (avoid const_cast) | ||
| std::string param_copy = m_param; | ||
| std::string str_copy = str; | ||
|
|
||
| std::vector<char*> argv; | ||
| argv.push_back(param_copy.data()); | ||
| argv.push_back(str_copy.data()); | ||
| argv.push_back(nullptr); | ||
|
|
||
| execvp(argv[0], argv.data()); |
There was a problem hiding this comment.
In init(), the script/executable path is resolved into m_file via utils::find_resource(), but the non-Lua execution path still executes m_param. This can execute a different binary (PATH lookup) or fail when the resource was found relative to the config. Prefer executing the resolved path (m_file) and use execv() (or execve()) rather than execvp() to avoid PATH ambiguity.
| while ((count = read(pipefd[0], buff.data(), buff.size())) > 0) { | ||
| s.write(buff.data(), count); | ||
| } | ||
|
|
||
| close(pipefd[0]); | ||
| waitpid(pid, nullptr, 0); | ||
|
|
There was a problem hiding this comment.
The read()/waitpid() handling should be robust against EINTR and should check the child exit status. As written, an interrupted read() will stop early and waitpid() ignores failures/status, so exec failures or signal termination may be silently treated as 'no match'. Consider retrying on EINTR and returning false (and/or logging) when the child exits non-zero.
| if (const std::string res = s.str(); | ||
| res.size() > 1 && res[0] != '1') { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
|
|
There was a problem hiding this comment.
The structured if statement here is mis-indented (and the closing brace alignment is off), which is likely to trip the repo's coding-style checks and makes the control flow harder to read. Please reformat this block so the condition, braces, and returns follow the surrounding indentation conventions.
| FILE *in; | ||
| std::array<char, 512> buff{}; | ||
| std::stringstream s; | ||
| std::string res; |
There was a problem hiding this comment.
In the Windows fallback branch, std::string res; is now unused because a new res is declared in the structured if statement below (shadowing the earlier variable). This can introduce unused-variable warnings; remove the outer res declaration or reuse it instead of redeclaring.
| std::string res; |
| if (const std::string res = s.str(); | ||
| res.size() > 1 && res[0] != '1') { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This structured if statement is also mis-indented in the Windows fallback path. Please reformat it to match the surrounding style (condition alignment, brace placement, and indentation of the return).
This PR proposes a security hardening improvement to the InspectFile operator external command execution path.
Currently, InspectFile::evaluate constructs a command string and invokes it using popen(), which executes via the system shell.
While current usage paths (e.g., FILES_TMPNAMES) pass engine-generated sanitized filenames and are not attacker-controlled, invoking a shell with concatenated arguments is a fragile pattern and increases risk if future variable sources expand.
This PR starts discussion toward replacing shell-based execution with argument-safe process execution (execve/spawn with explicit argv array), removing shell interpretation from the execution path.
Security benefits:
This PR currently adds a security hardening note and opens discussion on the preferred cross-platform implementation approach for v2 and v3.